Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: handle static assets in case-sensitive manner #10475

Merged
merged 5 commits into from
Nov 12, 2022

Conversation

benmccann
Copy link
Collaborator

@benmccann benmccann commented Oct 14, 2022

Description

Enforce case sensitivity so that your code works if you develop on Mac / Windows and then deploy to Linux. It also becomes easier to work on a code base where people on the team are using different OS.

Closes #9260

Additional context

Discussed in #contributing on Oct 14: https://discord.com/channels/804011606160703521/804439875226173480/1030501340373336144


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@benmccann
Copy link
Collaborator Author

Hmm. I might need some help with this one. The test passes on Linux, which is what I run, so this will be very difficult for me to get that test working since I don't have a way to reproduce the error

@dominikg
Copy link
Contributor

the check "file exists with correct case" on case insensitive fs may be usefule outside of the static middleware so maybe move it to a util and add a unit test too?

there is

export const isCaseInsensitiveFS = testCaseInsensitiveFS()
that also uses existsSync to test for case insensitivity.

The current implementation recurses to check all parent directory names separately, doing readDir calls to list files. This could be a bit more expensive on the disk operation side, so may be worth caching the positive results in a set?

@dominikg
Copy link
Contributor

The error response is

{
  name: 'RAW.js',
  status: 500,
  text: '\n' +
    '        <!DOCTYPE html>\n' +
    '        <html lang="en">\n' +
    '          <head>\n' +
    '            <meta charset="UTF-8" />\n' +
    '            <title>Error</title>\n' +
    '            <script type="module">\n' +
    "              import { ErrorOverlay } from '/@vite/client'\n" +
    '              document.body.appendChild(new ErrorOverlay({"message":"Failed to load url /RAW.js (resolved id: /RAW.js). This file is in /public and will be copied as-is during build without going through the plugin transforms, and therefore should not be imported from source code. It can only be referenced via HTML tags.","stack":"    at loadAndTransform (file:///C:/Users/IEUser/dev/vite/packages/vite/dist/node/chunks/dep-f1192f4f.js:37817:19)\\n    at processTicksAndRejections (node:internal/process/task_queues:96:5)","frame":""}))\n' +
    '            </script>\n' +
    '          </head>\n' +
    '          <body>\n' +
    '          </body>\n' +
    '        </html>\n' +
    '      '
}

which seems like some extra handling for script files in public dir.

If you use icon.png and ICON.png instead the test passes on windows at least

@benmccann
Copy link
Collaborator Author

Thank you so much for taking a look at this!

If you use icon.png and ICON.png instead the test passes on windows at least

Are you sure? I just changed it to that and the CI is failing still. Though now it gets a 200 rather than 500, which I suppose is an improvement.

It's curious because I stole this code from SvelteKit and it has a test for it that passes.

@dominikg
Copy link
Contributor

The previous fail was during test-serve, the current one is on test-build. Unfortunately i didn't run the full suite after finding the first problem. Sorry, 🙈
I guess the build tests use vite preview or another static server to serve the build output and that doesn't use the middleware you changed in this PR.

@benmccann
Copy link
Collaborator Author

Ah, thanks!! I really appreciate the help @dominikg! I've implemented it for preview as well so the tests are passing now!

@benmccann
Copy link
Collaborator Author

I moved the method to utils.ts as part of making it shared between dev and preview. I'm not sure it needs to be made more general until there's a usecase for it though.

I also leveraged the isCaseInsensitiveFS variable you mentioned to skip the check on Linux. I'm not too worried about optimizing performance beyond that though because it only affects dev/preview and not prod and this functionality has been in SvelteKit for a long time and has never caused any perf issues there.

@BenediktAllendorf
Copy link
Contributor

I had a look at the Todo about those double slashes:

First of all, the behaviour (double slashes) is not new. Apparently sirv() didn't care but shouldServe() does.

They occur because viteTestUrl is set here to something like 'http://localhost:4173/' (with trailing slash) and many (most? all?) URLs in the tests are build like await page.goto(viteTestUrl + '/something'), leading to duplicated slashes.

So either change all the tests not to include that slash, change viteTestUrl to not include the trailing slash, or make shouldServe() handle double slashes as well.

@patak-dev patak-dev added this to the 4.0 milestone Oct 21, 2022
@benmccann
Copy link
Collaborator Author

So either change all the tests not to include that slash, change viteTestUrl to not include the trailing slash, or make shouldServe() handle double slashes as well.

Thanks! I added it to shouldServe

@bluwy
Copy link
Member

bluwy commented Nov 4, 2022

We discussed this in the last team meeting and agree with this. Just needs a review and it's good to go.

packages/vite/src/node/utils.ts Outdated Show resolved Hide resolved
packages/vite/src/node/utils.ts Outdated Show resolved Hide resolved
Comment on lines 1202 to 1205
const pathname = decodeURIComponent(
new URL(url.startsWith('//') ? url.substring(1) : url, 'http://example.com')
.pathname
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like sirv is able to handle // because it uses @polka/url. We could probably use that too as it caches the result to req, but also something we can improve later.

benmccann and others added 2 commits November 11, 2022 08:23
Co-authored-by: Bjorn Lu <bjornlu.dev@gmail.com>
@benmccann
Copy link
Collaborator Author

Thanks for the review @bluwy! I've addressed your comments

Copy link
Member

@patak-dev patak-dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Let's merge so we give more time to other projects to test this.

@patak-dev patak-dev merged commit c1368c3 into vitejs:main Nov 12, 2022
@benmccann benmccann deleted the case-sensitive branch November 12, 2022 19:49
@ElMassimo
Copy link
Contributor

ElMassimo commented Dec 9, 2022

Testing Vite 4 with îles.

An unintended effect of #10475 is that sirv will only receive requests to /existing if there's an existing/index.html file, while prior to the change sirv was able to resolve /existing to existing.html file.

This is important to be able to locally preview sites that will be deployed using cleanUrls in Vercel, or in Netlify.

Ideally, frameworks like îles should be able to override shouldServe, as it limits the usefulness of the preview command.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority)
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Enforce case sensitivity when serving static files
7 participants